Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR adds optional commit message inclusion for memory items. It introduces a new Changes
Sequence DiagramsequenceDiagram
participant Handler as PromptSubmitHandler
participant ConfigLoader as IHookConfigLoader
participant MemLoader as MemoryContextLoader
participant GitClient as GitClient
participant Formatter as ContextFormatter
Handler->>ConfigLoader: Load hook config
ConfigLoader-->>Handler: Return config (includeCommitMessages)
Handler->>MemLoader: load(memories, cwd, includeCommitMessages)
alt includeCommitMessages && gitClient exists
MemLoader->>GitClient: getCommitMessages(shas, cwd)
GitClient-->>MemLoader: Map<sha, {subject, body}>
else
MemLoader-->>MemLoader: Skip commit fetching
end
MemLoader-->>Handler: Return {memories, commitMessages}
Handler->>Formatter: format(memories, {commitMessages})
Formatter->>Formatter: Render commit subject & body per item
Formatter-->>Handler: Formatted context with commits
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/application/services/MemoryContextLoader.ts`:
- Line 71: The current extraction in MemoryContextLoader that builds shas from
memories uses filter(Boolean) which doesn't narrow types; replace that filter
with a proper type guard so the resulting shas is string[] (for example use a
predicate like (s): s is string => s !== undefined && s !== null or (s): s is
string => Boolean(s)). Update the expression that references shas (the const
shas = [...new Set(memories.map(m => m.sha).filter(Boolean))];) to use the
type-guarded filter so TypeScript knows shas contains only strings.
- Line 40: Replace the non-idiomatic inline type import by adding a top-level
import for IMemoryEntity from '../../domain/entities/IMemoryEntity' and then
change the cast sites (e.g., the `memories` assignment and the other occurrence
around line 67) to use the imported type (e.g., `readonly IMemoryEntity[]`)—look
for `memories` and any other `import('...').IMemoryEntity` uses in
MemoryContextLoader.ts and update them to reference the top-level IMemoryEntity
import.
In `@src/infrastructure/git/GitClient.ts`:
- Around line 276-342: The parsing in getCommitMessages can truncate commit
bodies if they contain the FIELD_SEP (GitClient.FIELD_SEP); change the splits to
limit to three fields so only the first two separators are honored (e.g., use
split with a limit of 3) when parsing both the batch records (records.map/for
loop that assigns sha, subject, body) and the per-commit fallback (where output
is split into subject and body) so the body retains any embedded separators;
update references around GitClient.RECORD_SEP, GitClient.FIELD_SEP, and the
getCommitMessages parsing logic accordingly.
| cwd?: string, | ||
| ): ReadonlyMap<string, ICommitMessage> { | ||
| // Collect unique SHAs from memories | ||
| const shas = [...new Set(memories.map(m => m.sha).filter(Boolean))]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a type guard for proper type narrowing.
filter(Boolean) removes falsy values at runtime but TypeScript doesn't narrow the resulting type. If sha can be undefined, the array type remains (string | undefined)[] rather than string[].
🛡️ Proposed fix for type safety
- const shas = [...new Set(memories.map(m => m.sha).filter(Boolean))];
+ const shas = [...new Set(memories.map(m => m.sha).filter((sha): sha is string => Boolean(sha)))];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shas = [...new Set(memories.map(m => m.sha).filter(Boolean))]; | |
| const shas = [...new Set(memories.map(m => m.sha).filter((sha): sha is string => Boolean(sha)))]; |
🤖 Prompt for AI Agents
In `@src/application/services/MemoryContextLoader.ts` at line 71, The current
extraction in MemoryContextLoader that builds shas from memories uses
filter(Boolean) which doesn't narrow types; replace that filter with a proper
type guard so the resulting shas is string[] (for example use a predicate like
(s): s is string => s !== undefined && s !== null or (s): s is string =>
Boolean(s)). Update the expression that references shas (the const shas =
[...new Set(memories.map(m => m.sha).filter(Boolean))];) to use the type-guarded
filter so TypeScript knows shas contains only strings.
| getCommitMessages(shas: readonly string[], cwd?: string): Map<string, { subject: string; body: string }> { | ||
| const result = new Map<string, { subject: string; body: string }>(); | ||
|
|
||
| if (shas.length === 0) return result; | ||
|
|
||
| // Deduplicate SHAs | ||
| const uniqueShas = [...new Set(shas)]; | ||
|
|
||
| // Use git log with specific SHAs to fetch all at once | ||
| // Format: SHA<FS>subject<FS>body<RS> | ||
| const format = ['%H', '%s', '%b'].join(GitClient.FIELD_SEP); | ||
|
|
||
| try { | ||
| // We use --no-walk to avoid following parents, just show the specified commits | ||
| const output = execFileSync( | ||
| 'git', | ||
| [ | ||
| 'log', | ||
| '--no-walk', | ||
| `--format=${GitClient.RECORD_SEP}${format}`, | ||
| ...uniqueShas, | ||
| ], | ||
| { | ||
| encoding: 'utf8', | ||
| cwd, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| maxBuffer: 10 * 1024 * 1024, | ||
| } | ||
| ).trim(); | ||
|
|
||
| if (!output) return result; | ||
|
|
||
| const records = output.split(GitClient.RECORD_SEP).filter(r => r.trim()); | ||
| for (const record of records) { | ||
| const fields = record.split(GitClient.FIELD_SEP); | ||
| const sha = fields[0] || ''; | ||
| const subject = fields[1] || ''; | ||
| const body = (fields[2] || '').trim(); | ||
|
|
||
| if (sha) { | ||
| result.set(sha, { subject, body }); | ||
| } | ||
| } | ||
| } catch { | ||
| // If batch fails, try individual lookups as fallback | ||
| for (const sha of uniqueShas) { | ||
| try { | ||
| const output = execFileSync( | ||
| 'git', | ||
| ['log', '-1', `--format=%s${GitClient.FIELD_SEP}%b`, sha], | ||
| { | ||
| encoding: 'utf8', | ||
| cwd, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| } | ||
| ).trim(); | ||
|
|
||
| const [subject, body] = output.split(GitClient.FIELD_SEP); | ||
| result.set(sha, { subject: subject || '', body: (body || '').trim() }); | ||
| } catch { | ||
| // Skip commits that can't be found | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Solid implementation with batch optimization and fallback.
The approach is sound: batch query with --no-walk for efficiency, deduplication via Set, and per-SHA fallback for resilience.
Minor edge case: If a commit body contains the field separator \x1f, the body would be truncated at line 333. This is extremely rare in practice (ASCII Unit Separator in commit messages), but for robustness you could use a limit parameter on split:
🔧 Optional fix for body containing field separator
- const [subject, body] = output.split(GitClient.FIELD_SEP);
+ const parts = output.split(GitClient.FIELD_SEP);
+ const subject = parts[0] || '';
+ const body = parts.slice(1).join(GitClient.FIELD_SEP).trim();The same pattern could be applied to the batch parsing at lines 310-313 if desired.
🤖 Prompt for AI Agents
In `@src/infrastructure/git/GitClient.ts` around lines 276 - 342, The parsing in
getCommitMessages can truncate commit bodies if they contain the FIELD_SEP
(GitClient.FIELD_SEP); change the splits to limit to three fields so only the
first two separators are honored (e.g., use split with a limit of 3) when
parsing both the batch records (records.map/for loop that assigns sha, subject,
body) and the per-commit fallback (where output is split into subject and body)
so the body retains any embedded separators; update references around
GitClient.RECORD_SEP, GitClient.FIELD_SEP, and the getCommitMessages parsing
logic accordingly.
There was a problem hiding this comment.
Pull request overview
This PR adds support for including commit message bodies alongside memories when they are surfaced in the prompt-submit hook. The implementation fetches commit messages in batch from Git, passes them through the context loading pipeline, and renders them inline with each memory in the formatted output.
Changes:
- Added
getCommitMessages()batch method to GitClient for efficient commit message retrieval - Extended MemoryContextLoader to optionally fetch and return commit messages alongside memories
- Updated ContextFormatter to render commit subjects and bodies (up to 3 lines) inline with memories
- Added
includeCommitMessagesconfig option to IPromptSubmitConfig (default: true)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/infrastructure/git/GitClient.ts | Implements getCommitMessages() with batch fetching and individual fallback |
| src/domain/interfaces/IGitClient.ts | Adds interface definition for getCommitMessages() method |
| src/domain/interfaces/IMemoryContextLoader.ts | Adds ICommitMessage type and commitMessages to result/options |
| src/domain/interfaces/IContextFormatter.ts | Adds commitMessages option for formatting |
| src/domain/interfaces/IHookConfig.ts | Adds includeCommitMessages field to IPromptSubmitConfig |
| src/application/services/MemoryContextLoader.ts | Implements commit message fetching with error handling |
| src/application/services/ContextFormatter.ts | Renders commit messages inline with memories |
| src/application/handlers/PromptSubmitHandler.ts | Loads config and passes includeCommitMessages option through pipeline |
| src/infrastructure/di/container.ts | Wires hookConfigLoader to PromptSubmitHandler |
| src/hooks/utils/config.ts | Sets default value for includeCommitMessages to true |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [subject, body] = output.split(GitClient.FIELD_SEP); | ||
| result.set(sha, { subject: subject || '', body: (body || '').trim() }); |
There was a problem hiding this comment.
The destructuring assignment only captures the first two elements when splitting on FIELD_SEP. If the commit body contains the FIELD_SEP character (ASCII 0x1F), the body will be truncated at that point. This should use the same index-based approach as the batch parsing to handle the body correctly: const fields = output.split(GitClient.FIELD_SEP); result.set(sha, { subject: fields[0] || '', body: fields.slice(1).join(GitClient.FIELD_SEP).trim() });
| const [subject, body] = output.split(GitClient.FIELD_SEP); | |
| result.set(sha, { subject: subject || '', body: (body || '').trim() }); | |
| const fields = output.split(GitClient.FIELD_SEP); | |
| const subject = fields[0] || ''; | |
| const body = fields.slice(1).join(GitClient.FIELD_SEP).trim(); | |
| result.set(sha, { subject, body }); |
|
Review comments addressed:
|
Add commit subject and body alongside memories in prompt-submit output. When memories are loaded, their associated commit messages are fetched and displayed inline, giving the AI richer context about past decisions. - Add getCommitMessages() to IGitClient for batch commit fetching - Extend IMemoryContextLoader result with commitMessages map - Update ContextFormatter to render commit subjects/bodies inline - Add includeCommitMessages config option (default: true) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101 AI-Decision: include commit message bodies in memory context (GIT-104). Add commit subject and body alongside memories in prompt-submit output. AI-Confidence: medium AI-Tags: application, handlers, services, domain, hooks, utils, infrastructure AI-Lifecycle: project AI-Memory-Id: c78152dc AI-Source: heuristic
Summary
getCommitMessages()to GitClient for batch fetching commit subjects and bodiesincludeCommitMessagesconfig option to IPromptSubmitConfig (default: true)When memories are surfaced in prompt-submit, their associated commit messages now appear inline:
Closes GIT-104
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit